-
Notifications
You must be signed in to change notification settings - Fork 684
Fix for null pointer dereference in jmem_heap_free_block #2440
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix for null pointer dereference in jmem_heap_free_block #2440
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add the reported issue as a test case.
Hi, While this change does prevent segfault, undefined behavior is still triggered at jerryscript/jerry-core/ecma/builtin-objects/typedarray/ecma-builtin-typedarray-prototype.c:598 This is because behavior of memcpy is undefined if the second argument is NULL. Running the example provided by me shows the following message: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain how this related to a memcpy?
The implementation of Current implementation (without this commit) in The default build for jerryscript enbales gcc optimisation |
0971d4d
to
b6daa0b
Compare
I've added an early return to resolve this issue, and the test case as well. |
This version no longer triggers undefined behavior. |
b6daa0b
to
58f2bd4
Compare
{ | ||
return ret_value; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need an extra newline.
jerry-core/jmem/jmem.h
Outdated
else \ | ||
{ \ | ||
JERRY_ASSERT (var_name ## ___size == 0); \ | ||
} \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change should be removed.
0e58db4
to
675f06e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
if (len == 0) | ||
{ | ||
return ret_value; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, why this is not before the define local array?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because in some cases we can not return with ECMA_VALUE_EMPTY
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so we return with a new typedarray?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually we return from a macro enclosed block, and that is not necessarily healthy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes but we only return from there when the length is 0, therefore the enclosing macro would basically do nothing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And yes, we have to give back a 0 length typedarray in that case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is we might change silently the macro and we have a hidden bug. Creating and returning with an empty array early should be easy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that is definitely right, it would just mean a bit of a code duplication there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The compiler should handle that. Since it is a single line, I would not even call it a duplication.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah you convinced me there, I'm updating the PR
675f06e
to
c2dd306
Compare
Fixes jerryscript-project#2435. JerryScript-DCO-1.0-Signed-off-by: Daniel Balla [email protected]
c2dd306
to
a4ee88c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Fixes #2435.
JerryScript-DCO-1.0-Signed-off-by: Daniel Balla [email protected]